Skip to content

Throw friendly errors#342

Merged
dwalton76 merged 4 commits intodevelopfrom
throw-friendly-errors
Sep 10, 2017
Merged

Throw friendly errors#342
dwalton76 merged 4 commits intodevelopfrom
throw-friendly-errors

Conversation

@WasabiFan
Copy link
Copy Markdown
Member

Fixes #331

It would be nice if we were able to adapt the message when you attempt to run for distance with a negative sign, but commands are read-only and I didn't see an easy way to pipe that through. Any suggestions for other properties?

robot@ev3dev:~$ python3
Python 3.5.3 (default, Jan 19 2017, 14:11:04)
[GCC 6.3.0 20170118] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ev3dev.ev3 as ev3
>>> m = ev3.Motor()
>>> m.connected
True
>>> m.speed_sp = -100000
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 210, in _set_attribute
    attribute.write(value.encode())
OSError: [Errno 22] Invalid argument

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 604, in speed_sp
    self._speed_sp = self.set_attr_int(self._speed_sp, 'speed_sp', value)
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 238, in set_attr_int
    return self._set_attribute(attribute, name, str(int(value)))
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 213, in _set_attribute
    self._raise_friendly_access_error(ex, name)
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 229, in _raise_friendly_access_error
    raise ValueError("The given speed value was out of range. Max speed: +/-" + str(max_speed)) from driver_error
ValueError: The given speed value was out of range. Max speed: +/-1050
>>> m.speed_sp = 1000000
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 210, in _set_attribute
    attribute.write(value.encode())
OSError: [Errno 22] Invalid argument

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 604, in speed_sp
    self._speed_sp = self.set_attr_int(self._speed_sp, 'speed_sp', value)
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 238, in set_attr_int
    return self._set_attribute(attribute, name, str(int(value)))
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 213, in _set_attribute
    self._raise_friendly_access_error(ex, name)
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 229, in _raise_friendly_access_error
    raise ValueError("The given speed value was out of range. Max speed: +/-" + str(max_speed)) from driver_error
ValueError: The given speed value was out of range. Max speed: +/-1050

@WasabiFan
Copy link
Copy Markdown
Member Author

Another potential candidate for friendlyifying:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "core.py", line 487, in position
  File "core.py", line 210, in get_attr_int
  File "core.py", line 192, in _get_attribute
OSError: [Errno 19] ENODEV

(instantiate motor, disconnect motor, access attribute... BAM! error.)

Comment thread ev3dev/core.py
raise ValueError("The given speed value was out of range") from driver_error
else:
raise ValueError("The given speed value was out of range. Max speed: +/-" + str(max_speed)) from driver_error
raise ValueError("One or more arguments were out of range or invalid") from driver_error
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not convinced a value error is more friendly than OSError to be fair. OSError is the thing that happens in the background, why do we want to 'protect' the user from this?

May be it would be better to at least re-raise the original (OSError) exception instead of generic 'One or more arguments were out of range or invalid' for unsupported attributes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, what would make the debugging easier?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not convinced a value error is more friendly than OSError to be fair. OSError is the thing that happens in the background, why do we want to 'protect' the user from this?

That the nice Python functions happen to be calling an operating system routine is an implementation detail. As far as the user is concerned, they called the function and provided an argument that was out-of-range; a ValueError is the Python concept that describes such an issue, and the message explains what went wrong. If the user is interested in the OSError, they can see it as the __cause__ of the ValueError (hence raise ... from).

For most casual programmers, the message OSError: [Errno 22] Invalid argument will be confusing -- it isn't natural to expect an OSError for this sort of scenario, and the message certainly isn't clear. Unless I'm directly consuming OS functions, I would generally expect an OSError to indicate an issue with the implementation of a library, not my own mistake.

May be it would be better to at least re-raise the original (OSError) exception instead of generic 'One or more arguments were out of range or invalid' for unsupported attributes?

See above; the inner exception is available, just within the user-friendly error that we throw.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, what would make the debugging easier?

The error tells you exactly what you did wrong; no question is left as to what was going on when the error occurred or what caused it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would make the debugging easier for me if the exception text always included

  • the name of the attribute we are trying to set
  • the value we are trying to set it to
  • the min/max or acceptable inputs for that attribute

that would be very useful

@dwalton76 dwalton76 merged commit 22d493e into develop Sep 10, 2017
@WasabiFan WasabiFan deleted the throw-friendly-errors branch February 14, 2018 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants